Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: repo improvement suggestions #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

comilao
Copy link
Contributor

@comilao comilao commented Apr 26, 2024

What 💻

This PR tries to make some improvement suggestions to this project based on Python Best Practices for a New Project in 2021 and my Python experience with this blog article.

There is only one additional dev dependencies introduced pre-commit, it can be installed into Git Hooks and run automatically before any git commit locally. Its configuration file is .pre-commit-config.yaml, and I have configured some hooks such as no trailing-whitespace and add new line at the end of file. Other hooks are isort, flake8, black, mypy. These hooks are installed by pre-commit at a separate location on the machine and will not interfere with dependency of this repo.

  • isort groups imports into 3 categories, packages imported from Python own library, external library (web3) and other local imports. Within each category, it sorts imports in alphabetical order.
  • flake8 is a linter, it can detect things like unused imports, variables; variable use before its creation and etc
  • black is a formatter
  • mypy is a static type checker for Python

Currently, within .pre-commit-config.yaml, it does not check any files, you can learn more about the exclude regex here. With refactor going on, we can change the exclude part and check those files slowly.

I would also like to explain some changes in this commit:
validate.yml will be using pre-commit to do a comprehensive check. ref
pyproject.toml has configs for isort and black. (NOTE: I personally prefers line length 99, as nowadays computer screen is quite wide, but if you don't like it, I can change it to the default 88)
setup.cfg has configs for flake8 and mypy, mypy config ref

Why ✋

When I first start to use this repo, I noticed the Any type is returned everywhere, mainly because of #63 , it made the developing experience very bad. Then I dig deeper, and noticed many unused imports, wrong type hints, and some wrong typing issue that could be caught with a linter. I would like to propose my Python practice to this repo. The better this repo is, the smoother my developing experience will be.

Evidence 📷

Include screenshots, screen recordings, or console output here demonstrating that your changes work as intended

Notes 📝

  • Any notes/thoughts that the reviewers should know prior to reviewing the code?
    Sorry for this long PR, basically I would like to try my best to make this repo better, I finally found an open-source project that I feel I can contribute a lot.
  • If you are okay with these changes, I can also edit the CONTRIBUTING.md with a notes on pre-commit hook installation and usage.
  • BTW, if you accepted this PR, I can help to do the refactoring work with some minor parts in the repo first, such as zksync2/account, zksync2/core, zksync/manage_contracts, zksync/signer and zksync/transaction. I am thinking of using existing test cases to check the correctness of the refactoring and add more test cases accordingly.

@comilao comilao requested a review from a team as a code owner April 26, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant